Is there a reason you're using .Result instead of using async/await?

To make it synchronous since I moved it to the last possible point (when a .tests.ps1 file with a Describe block gets opened). If I did not call .Result (i.e. make it async) then it would default to the old Pester syntax in the first 1-2 seconds until the value has been determined (which people did not like in the first version of the PR). It is still using Task APIs because the PSCommand APIs that were suggested are all async.

What I'm worried about here is...

Lets look at the PSModulePath:

/Users/tyler/.local/share/powershell/Modules                       # user level
/usr/local/share/powershell/Modules                                # all users level
/usr/local/microsoft/powershell/6-preview/Modules                  # internal

let's say they have version 4.5 in their user module path ( but 4.6 in their allusers module path

This function will return true but 4.5 is what will get imported when doing:

Import-Module Pester
or during module auto-loading

I think we should only check the first item in the result. This should be the Pester that would get imported.

Is that really the behaviour of PowerShell to not use the latest available version? Can you please point to some docs or code for my own education. How do you know that just looking at the first item is enough? Get-Module really needs to be improved otherwise to be more useful/predictive...
If that is really the case, then should we not rather call (Get-Command Invoke-pester).Version instead (which has the drawback of being 3 times more expensive for some reason...)?

Yes that's the behavior of PowerShell. I'm unaware of any docs out in the wild (cc @SteveL-MSFT or @joeyaiello maybe?).

The PSModulePath acts the same way that the PATH environment variable does - it goes down the path looking for the command you want to run, then executes the first one it finds.

A simple test:

→ Install-Module Pester -Scope AllUsers -Force

→ Install-Module Pester -Scope CurrentUser -Force -MaximumVersion 4.5

→ Get-Command Invoke-Pester

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Function        Invoke-Pester                                      4.5.0      Pester

→ Get-Module -ListAvailable Pester


    Directory: /Users/tyler/.local/share/powershell/Modules

ModuleType Version    Name                                PSEdition ExportedCommands
---------- -------    ----                                --------- ----------------
Script     4.5.0      Pester                              Desk      {Describe, Context, It, Should…}

    Directory: /usr/local/share/powershell/Modules

ModuleType Version    Name                                PSEdition ExportedCommands
---------- -------    ----                                --------- ----------------
Script     4.6.0      Pester                              Desk      {Describe, Context, It, Should…}

How do you know that just looking at the first item is enough?

Because when you do Get-Module the first item written to the pipeline is first on the PSModulePath. That might not be correct though... Get-Command is probably better for this.

Hmm, yes, this sounds like Get-Command is probably the better option. I guess we win the investment back later when Invoke-Pester actually gets called. Personally I would keep it in a non-blocking task though and just have it behave like the old system in the first 2 seconds of opening a Pester file...

Yeah... can @rjmholt and @SeeminglyScience help me understand why this should be synchronous? I would think non-blocking would be better here.

AFAICT the bulk of the Pester code lens implementation is sync (other than LSP message reading/writing). This code is a bit old so maybe we weren't doing as much async then (??) or it could be that the specific symbol we are looking for results in a "fast enough" AST search.

One issue I have with this approach is that it doesn't take into account that I might have already imported an older version of Pester e.g. Import-Module Pester -RequiredVersion 4.1.1. So we should check Get-Module Pester first to see if the module is already loaded before getting the list of Pester module.

Another thing to consider is this feature seems to be implemented before PS Core was shipped. That is, Windows PowerShell comes with Pester but not PS Core - right? But this feature is still enabled resulting in this user exp:

Invoke-Pester : The term 'Invoke-Pester' is not recognized as the name of a cmdlet, function, script file, or operable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Perhaps we should consider shipping Pester with the extension like we do with PSSA? That would ensure this feature works on a vanilla PS Core install. This would ensure we have loaded a minimum version of Pester - like we do for PSSA. But we still allow folks to install later versions of Pester and those would get loaded. Again, very similiar to how we load PSSA.

@rkeithhill you touch on a lot of great points.

1. I think we should make this Async. Lets take the Lazy<bool> and turn it into a Lazy<Task<bool>> and lazily await the value. Shouldn't be too hard to plumb that. The original caller of this function is async already.

we should check Get-Module Pester first to see if the module is already loaded before getting the list of Pester module

This makes sense. But Get-Command Invoke-Pester should cover this if the module is already imported, right? Get-Command Invoke-Pester should be all we need, I think.

Perhaps we should consider shipping Pester with the extension like we do with PSSA?

Some people rely on specific version of Pester. PowerShell itself relied on a specific version of Pester for a while in preparation for Core support from Pester.

I don't think we can add Pester into PSES and grab the latest version on the PSModulePath because I don't want to hurt customers by forcing a newer version of Pester on them and all of a sudden their CI and their local are different.

PSSA does fall in that camp, a bit... but PSSA is more for local analysis than anything else, I feel.

@TylerLeonhardt I agree with point 2 and 3. Regarding point 2, we could potentially offer to install the latest version of Pester as a 3rd option in the dialog, but I see this as an optional, nice to have feature that can happen after this PR.

Regarding point 1: I am not sure if we are on the same page: At the moment, when GetPesterLens gets called (which is when a .tests.ps1 file gets opened and is therefore as late as possible), then we need to know the version of Pester already and have to wait for the result. When I first opened the PR I ran the check in a background thread (Task.Run without .Result), which meant that until it comes back, it reports false (which is what people criticised at first). I can change it back to this, but I do not need to make it an Lazy<Task<bool>> for that, I'd just remove the blocking call to .Result. Therefore it does not matter at all if the function that we call is already async or not, the reason why I have to use Task.Run in the first place is because the available Execute APIs on the recommended PSCommand class only have async methods,
If you really wanted true async, then we'd have to fix calling methods up to 3 layers above to make it async top to bottom (which is quite a bit more involved for this change...), otherwise you wouldn't get any real benefit from it and it.

I made it use Get-Command now, which makes the code lens appear a second later due to it being more expensive and I also implemented async-await all the way to the top in a minimum viable way. Can you please re-review?

You made this function async, but didn't add any await statements so async isn't needed here.

I had to change it here as well due to the signature change in the interface. The PesterCodeLensProvider properly implements async-await, which is what counts

Interesting! Typically the compiler yells if you have an async but no await but I guess since the interface requires it, the compiler doesn't yell.

Yep, it emits a warning though that this function won't run async as expected

You could changed the method to return:

return await Task.FromResult(acc.ToArray());

to make it happy I think.

I don't think this is a good idea because you might as well just suppress the warning. The compiler is happy, it is just kind to let us know that this other implementation will actually run synchronously despite the async declaration, Task.FromResult will still make it run sync but add unnecessary overhead. The warning is good to have because it is a way of tracking possible future improvement and also letting methods being called from this method know that they will actually need to adapt on a higher level if someone made them truly async as well